Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

repro: findComponent fails inside a Suspense #88

Closed

Conversation

cexbrayat
Copy link
Member

This adds a failing unit-test to showcase how findComponent fails if the search component is inside a Suspense.
I was naively expecting it to work, do you think we should support this?

@lmiller1990
Copy link
Member

lmiller1990 commented Apr 24, 2020

I have written tests in my other apps that work fine w/ Suspense. I think this is related to findComponent.

We should support this. findComponent needs to work 100% of the time in all scenarios to be supported. If we can't accomplish this, then we should not include it. Alternatively, we throughly document what it can do and the associated restrictions. I will play around with this now.

@dobromir-hristov may have some ideas, he knows more about how findComponent works than I do.

Edit: looks Suspense does not have child components in the same way that a regular VNode does. Hm.

@lmiller1990 lmiller1990 added the bug Something isn't working label Apr 24, 2020
@lmiller1990
Copy link
Member

lmiller1990 commented Apr 24, 2020

Edit edit: the blow by blow commentary continues - children in a Suspense are not an array, but a key:value pair:

{
      default: [Function: renderFnWithContext],
      fallback: [Function: renderFnWithContext],
      _: 1
}

This should be fixable. I tried turning these guys into an array but that did not work. I also tried doing findComponent({ name: 'Async' }) and adding a name: 'Async' but this also did not work.

I am worried what other edges cases are involved here, though. Excellent work catching this early. I definitely recommend asserting against the DOM where possible, as opposed to components.

@lmiller1990
Copy link
Member

As a quick sanity check, I added a test for <Teleport /> and it worked.

@dobromir-hristov
Copy link
Contributor

Done. Suspense is pretty much still Alpha and not complete.

@lmiller1990
Copy link
Member

wow, nice @dobromir-hristov. I tried something like that but it did not work 🤔

Great!

@lmiller1990
Copy link
Member

lmiller1990 commented Apr 25, 2020

Actually @dobromir-hristov close, but not quite there.

I changed the test:

    const wrapper = mount(SuspenseComponent)
    expect(wrapper.findComponent(AsyncComponent).exists()).toBe(false)
    await nextTick()
    expect(wrapper.findComponent(AsyncComponent).exists()).toBe(true)

This is failing on the first assertion - because both default and fallback are in the Suspense tree. Any way to know which one is visible (other than the obvious solution of the DOM).

@dobromir-hristov
Copy link
Contributor

hm... Will check it out.

@cexbrayat
Copy link
Member Author

@dobromir-hristov @lmiller1990 Where are we on this PR?

@dobromir-hristov
Copy link
Contributor

I played with it but got nowhere... Will try again this week.

@cexbrayat
Copy link
Member Author

As this was bugging me, I read the code of vue-next and now understand better how suspense is implemented.
Based on your feedback, I opened #168 (I did not copy your Teleport test @lmiller1990 , feel free to open a PR to add it!) if you have time to review

@cexbrayat cexbrayat closed this Jul 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants